refactor(appkit): own asUser OBO wrapping in Plugin proxy#385
refactor(appkit): own asUser OBO wrapping in Plugin proxy#385MarioCadenas wants to merge 1 commit into
Conversation
Move the OBO-side exports() wrapping out of AppKit#wrapWithAsUser and into Plugin.asUser's proxy. The proxy now intercepts reads of `exports` and returns the underlying exports() result with every function pre-wrapped in the user-context scope. The cross-file USER_CONTEXT_SYMBOL bridge between plugin.ts and appkit.ts is removed, and wrapExportsInUserContext is deleted from appkit.ts. The two inline Proxy literals in Plugin.asUser (real OBO and dev-mode fallback) collapse into a single _createAsUserProxy(wrapCall) factory that takes a per-call wrap strategy. The real OBO branch uses runInUserContext(userContext, ...); the dev fallback uses otelContext.with(DEV_OBO_FALLBACK_KEY=true, ...). Behavior preserving: full appkit test suite passes and typecheck is clean. Adds a focused 26-test file covering proxy mechanics: real OBO method calls, exports() interception (class methods, arrows, nested plain objects, class-instance values not recursed, callable exports, null/undefined), AsyncLocalStorage propagation across Promise.all, concurrent user isolation, error cleanup, dev-fallback exports(), and escape-the-proxy boundaries (returned functions not auto-wrapped, `return this` returns the unwrapped target). Also drop the now-unused UserContext type re-export from the internal context barrel (not part of the public package entry). Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
pkosiec
left a comment
pkosiec
left a comment
There was a problem hiding this comment.
Nice one! Tested Lakebase OBO - still works 👍
Before merge, please take a look at the agentic review summary.
There was a problem hiding this comment.
Code Review: PR #385 — refactor(appkit): own asUser OBO wrapping in Plugin proxy
Context
PR #385 moves OBO-side exports() wrapping out of AppKit#wrapWithAsUser and into Plugin.asUser's proxy. The cross-file USER_CONTEXT_SYMBOL bridge is removed, and two inline Proxy literals (real OBO + dev fallback) collapse into a single _createAsUserProxy(wrapCall) factory. A 26-test file covering proxy mechanics is added.
Scope: 4 files changed in packages/appkit/ — context/index.ts, core/appkit.ts, plugin/plugin.ts, plugin/tests/asUser-proxy.test.ts (new)
Reviewers dispatched: correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, reliability, api-contract
Review Findings
P2 — Moderate
| # | File | Issue | Reviewer(s) | Confidence | Route |
|---|---|---|---|---|---|
| 1 | packages/appkit/src/plugin/plugin.ts:52 |
Duplicate isPlainObject implementation. Identical logic exists at plugin.ts:52 (module function) and appkit.ts:180 (static method). Two sources of truth for the same check — if logic needs to change, both must be updated independently. |
maintainability | 0.95 | manual -> human |
| 2 | packages/appkit/src/plugin/plugin.ts:65 |
wrapExportFunctions mutates input object in-place. Currently safe because exports() returns a fresh object per call, but fragile if a plugin ever caches the result. The docstring says "Mutates and returns" but the wrap naming suggests a non-mutating operation. |
maintainability | 0.80 | advisory -> human |
| 3 | packages/appkit/src/plugin/plugin.ts:75 |
No circular reference protection in wrapExportFunctions. Recursive descent into nested plain objects has no cycle detection. Extremely unlikely with real plugin exports (shallow structures, isPlainObject rejects arrays/class instances), but undocumented limitation. |
testing | 0.70 | advisory -> human |
P3 — Low
| # | File | Issue | Reviewer(s) | Confidence | Route |
|---|---|---|---|---|---|
| 4 | packages/appkit/src/plugin/plugin.ts:70 |
Redundant Object.hasOwn() check. Object.keys() on line 69 already returns only own enumerable keys, so the hasOwn guard on line 70 can never be true — dead code. |
testing | 0.95 | safe_auto -> review-fixer |
Pre-existing (not introduced by this PR)
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| — | packages/appkit/src/core/appkit.ts:172 |
(plugin as any).asUser(req).exports() cast loses type safety. Pre-existing pattern — the old code also used (plugin as any) with USER_CONTEXT_SYMBOL. |
kieran-typescript | 0.92 |
Testing Gaps
- No test for
exports()returning non-plain, non-function values (Map, Set, Array as top-level return) — the fallthrough atplugin.ts:502is uncovered - No test for deeply nested exports (5+ levels) to confirm recursion terminates cleanly
- No integration test at AppKit layer for when
plugin.exports()throws duringwrapWithAsUser wrapExportFunctionsandisPlainObjecthave no isolated unit tests (only tested indirectly through proxy integration)
Coverage Notes
- Suppressed: 0 findings below confidence threshold
- Learnings: No
docs/solutions/directory exists — no institutional learnings to surface - Agent-Native: No gaps — this is an internal refactor with no new user-facing features
- Schema Drift / Deployment: N/A (no migrations)
- Failed reviewers: 0 of 9
Verdict: Ready to merge (with optional improvements)
The core refactoring is sound — consolidating two Proxy literals into _createAsUserProxy(wrapCall) is a clear improvement that eliminates the cross-file USER_CONTEXT_SYMBOL bridge. The 26-test file is thorough and well-organized, covering real OBO, dev fallback, exports interception, async propagation, concurrent user isolation, and boundary cases.
No P0 or P1 issues found in the PR's committed changes. The findings below are improvements, not blockers.
Improvement Plan
Fix 1: Remove redundant Object.hasOwn() check (P3)
File: packages/appkit/src/plugin/plugin.ts:70
Remove the dead if (!Object.hasOwn(exports, key)) continue; guard since Object.keys() on line 69 already returns only own enumerable keys.
Optional improvements (not blocking merge)
-
Deduplicate
isPlainObject— Extract to a shared utility (e.g.packages/appkit/src/utils.ts) or haveplugin.tsimportAppKit.isPlainObjectfromappkit.ts. Both files (plugin.ts:52andappkit.ts:180) have identical implementations. -
Add test for non-plain-object top-level exports — Add a test case where
exports()returns a Map or Array as the top-level value, verifying it's returned as-is without wrapping (theplugin.ts:502fallthrough path). -
Add test for mixed async/sync exports — Verify that an exports object with both async and sync functions preserves their semantics when wrapped.
Verification
After making changes:
pnpm build && pnpm test && pnpm check:fix && pnpm -r typecheck2 participants
Move the OBO-side exports() wrapping out of AppKit#wrapWithAsUser and into Plugin.asUser's proxy. The proxy now intercepts reads of
exportsand returns the underlying exports() result with every function pre-wrapped in the user-context scope. The cross-file USER_CONTEXT_SYMBOL bridge between plugin.ts and appkit.ts is removed, and wrapExportsInUserContext is deleted from appkit.ts.The two inline Proxy literals in Plugin.asUser (real OBO and dev-mode fallback) collapse into a single _createAsUserProxy(wrapCall) factory that takes a per-call wrap strategy. The real OBO branch uses runInUserContext(userContext, ...); the dev fallback uses otelContext.with(DEV_OBO_FALLBACK_KEY=true, ...).
Behavior preserving: full appkit test suite passes and typecheck is clean. Adds a focused 26-test file covering proxy mechanics: real OBO method calls, exports() interception (class methods, arrows, nested plain objects, class-instance values not recursed, callable exports, null/undefined), AsyncLocalStorage propagation across Promise.all, concurrent user isolation, error cleanup, dev-fallback exports(), and escape-the-proxy boundaries (returned functions not auto-wrapped,
return thisreturns the unwrapped target).Also drop the now-unused UserContext type re-export from the internal context barrel (not part of the public package entry).